Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Separate security policy and rule #1056

Merged
merged 2 commits into from
Dec 31, 2023
Merged

Separate security policy and rule #1056

merged 2 commits into from
Dec 31, 2023
+1,405 −108

Conversation

GraysonWu
Copy link
Collaborator

@GraysonWu GraysonWu commented Dec 6, 2023 •
edited
Loading

Fixes #777
Fixes #1043

@GraysonWu GraysonWu force-pushed the separate-rule branch 2 times, most recently from eaeba4e to 149dc4e Compare December 7, 2023 22:52
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu changed the title [WIP]Separate security policy and rule Separate security policy and rule Dec 8, 2023
@annakhm
Copy link
Collaborator

annakhm commented Dec 11, 2023

Hi @GraysonWu, I don't see a use case for rule data source - can you suggest where it might be needed?

annakhm
annakhm reviewed Dec 11, 2023
View reviewed changes
nsxt/policy_common.go Outdated Show resolved Hide resolved
annakhm
annakhm reviewed Dec 11, 2023
View reviewed changes
nsxt/policy_common.go Outdated Show resolved Hide resolved
@2ez4szliu
Copy link
Contributor

Hi @GraysonWu, I don't see a use case for rule data source - can you suggest where it might be needed?

I think it might be related to Issue 1043 #1043
But seems a resource for rules can already satisfy the user's need.

annakhm
annakhm reviewed Dec 11, 2023
View reviewed changes
nsxt/resource_nsxt_policy_security_policy_no_rule.go Outdated
}

func resourceNsxtPolicySecurityPolicyNoRuleCreate(d *schema.ResourceData, m interface{}) error {
connector := getPolicyConnector(m)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to parametrize the CRUD to functions to either include or exclude the rule, in order to reuse the common code with the other policy resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could make sense but I think it might deserve a separate PR to do so, including switch from using H-API to using security policy API for nsxt_policy_security_policy

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that securityPolicy's API couldn't delete rules. So to let parent_security_policy and security_policy share the common code I changed to use H-API for the parent_security_policy with the latest commit. It will make more sense to conduct in this PR instead of another one.
And, as for the other policy resource, did you mean gateway_policy and predefined_security_poilcy as well? From my understanding, predefined_security_policy has a different schema than security_policy. And gateway_policy, although sharing most of the schema, is a different model. I'm not sure merging those two resources could share much common code. Seems making them self-contained is a better option.
Glad to hear your thoughts!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify what you mean by lack of support for deleting rules from the API - I see this in the SDK.
Regardless, follow up PR for optimizing's the code is fine by me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion.
I mean I couldn't use the security policy client to delete the rule belonging to it. If we use the SDK for Rule to manage security_policy, not the parent one, we have to make several additional API calls for the rules, instead of one API call as H-API does. I'm a little bit concerned about the performance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when you fire the DELETE api call for the policy, all of its rules will be deleted - no need to delete them beforehand. What that your concern?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean for example, there is a policy that has 200 rules and we want to delete 100 rules.

annakhm
annakhm reviewed Dec 11, 2023
View reviewed changes
nsxt/provider.go Outdated
@@ -437,6 +438,8 @@ func Provider() *schema.Provider {
"nsxt_edge_high_availability_profile": resourceNsxtEdgeHighAvailabilityProfile(),
"nsxt_policy_host_transport_node_collection": resourceNsxtPolicyHostTransportNodeCollection(),
"nsxt_policy_lb_client_ssl_profile": resourceNsxtPolicyLBClientSslProfile(),
"nsxt_policy_security_policy_rule": resourceNsxtPolicySecurityPolicyRule(),
"nsxt_policy_security_policy_no_rule": resourceNsxtPolicySecurityPolicyNoRule(),
Copy link
Collaborator

@annakhm annakhm Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that nsxt_policy_security_policy_no_rule is confusing, especially next to nsxt_policy_security_policy_rule.
I don't have a much better option, but lets not give up on better naming yet :)
nsxt_policy_empty_security_policy? nsxt_policy_parent_security_policy?
There is also an option to remove the first policy part, since non-policy resources are deprecated by now.
This would result in nsxt_security_policy and nsxt_security_policy_rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to nsxt_policy_parent_security_policy. Ideas are welcome!

@vmwclabot
Copy link
Member

@GraysonWu, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@vmwclabot vmwclabot added the dco-required DCO Required label Dec 13, 2023
@vmwclabot vmwclabot removed the dco-required DCO Required label Dec 13, 2023
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu
Copy link
Collaborator Author

GraysonWu commented Dec 13, 2023 •
edited
Loading

Hi @GraysonWu, I don't see a use case for rule data source - can you suggest where it might be needed?

Not for now. As @2ez4szliu said, a data source might be needed for issue #1043. Considering a resource should be enough, let me remove it.

@GraysonWu
Copy link
Collaborator Author

/test-all

1 similar comment
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu
Copy link
Collaborator Author

/test-all

annakhm
annakhm reviewed Dec 20, 2023
View reviewed changes
nsxt/policy_common.go Outdated Show resolved Hide resolved
@GraysonWu
Copy link
Collaborator Author

/test-all

1 similar comment
@annakhm
Copy link
Collaborator

annakhm commented Dec 27, 2023

/test-all

@GraysonWu
Copy link
Collaborator Author

/test-all

annakhm
annakhm approved these changes Dec 28, 2023
View reviewed changes
Copy link
Collaborator

@annakhm annakhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@GraysonWu GraysonWu merged commit b8b33d1 into vmware:master Dec 31, 2023
5 of 6 checks passed
@Nacymus
Copy link

Nacymus commented Feb 29, 2024 •
edited
Loading

Hello @GraysonWu , @annakhm

Why is nsxt_policy_security_policy_rule still appears as a beta resource in the latest release (3.5) documentation ? I tried CRUD operations on it, it works wonders.

@annakhm
Copy link
Collaborator

annakhm commented Feb 29, 2024

Hello @Nacymus, glad to hear it works for you! We also feel good about the quality of this resource, but haven't run the full set of tests on it yet. We expect to promote this from beta on next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annakhm annakhm annakhm approved these changes

@ksamoray ksamoray Awaiting requested review from ksamoray

@2ez4szliu 2ez4szliu Awaiting requested review from 2ez4szliu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@GraysonWu @annakhm @2ez4szliu @vmwclabot @Nacymus